Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modal content (fixes #113) #151

Merged
merged 36 commits into from
Feb 19, 2020
Merged

Conversation

OtisRed
Copy link
Contributor

@OtisRed OtisRed commented Aug 8, 2019

Complete although a lil bit messy in terms of css, so any suggestions (@Loczi94) in that regard would be great.

Manual testing:

  1. docker up the app
  2. click any of the projects
  3. check if it renders proper details (use Volontulo - the rest doesn't have all the data filled yet)
  4. check if all buttons work
  5. check the console for errors

@OtisRed
Copy link
Contributor Author

OtisRed commented Aug 15, 2019

@stanislawK @jacekkalbarczyk it's a workin prototype, although unstylized (I'm not really good with scss). You will find the most representative example under Volontulo project.

@OtisRed OtisRed changed the title Modal content (fixes #113) [WIP] Modal content (fixes #113) Aug 24, 2019
Copy link
Contributor

@Loczi94 Loczi94 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job!
I left some comments mostly regarding things that are not needed.

frontend/src/components/ModalContent.vue Show resolved Hide resolved
frontend/src/assets/images/copyleft.svg Outdated Show resolved Hide resolved
frontend/src/components/ModalContent.vue Outdated Show resolved Hide resolved
frontend/src/components/ModalContent.vue Outdated Show resolved Hide resolved
frontend/src/components/ModalContent.vue Outdated Show resolved Hide resolved
@OtisRed OtisRed changed the title Modal content (fixes #113) Modal content (fixes #113) [WIP] Oct 23, 2019
@OtisRed OtisRed changed the title Modal content (fixes #113) [WIP] Modal content (fixes #113) Nov 20, 2019
@OtisRed
Copy link
Contributor Author

OtisRed commented Nov 20, 2019

@Loczi94 the code after merging master and refactoring that came afterwards :D

magul
magul previously requested changes Dec 1, 2019
Copy link
Member

@magul magul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few issues to be addressed (I haven't checked the whole change).

One additional question (albeit it's not the core of these changes) - I did a research and figure out that our SVG file with a magnifier comes from here. I see that Fabián hadn't provided a way to feasible install them from npm. What do you think about migrating to free tier of font-awasome (which we can install using npm install @fortawesome/fontawesome-free) as there's pretty similar icon, that we could use?

frontend/src/components/OurProjects.vue Outdated Show resolved Hide resolved
frontend/src/components/OurProjects.vue Outdated Show resolved Hide resolved
frontend/src/components/OurProjects.vue Outdated Show resolved Hide resolved
@OtisRed
Copy link
Contributor Author

OtisRed commented Dec 6, 2019

@magul if you check #97 there is already a task for that particular issue, although the rationale was different as current picture is shared on cc by 4.0 license instead of MIT or CC0.

As I understand you are not a fan of putting things like this in static assets?

Also I addressed the rest of the comments, thanks for them. If you're satisfied please approve not to block that PR :)

frontend/src/components/ModalContent.vue Outdated Show resolved Hide resolved
frontend/src/components/ModalContent.vue Outdated Show resolved Hide resolved
frontend/src/components/ModalContent.vue Outdated Show resolved Hide resolved
frontend/src/components/ModalContent.vue Outdated Show resolved Hide resolved
frontend/src/components/OurProjects.vue Outdated Show resolved Hide resolved
@magul magul dismissed their stale review January 22, 2020 19:18

dismiss review

@OtisRed
Copy link
Contributor Author

OtisRed commented Feb 8, 2020

@stanislawK from my part it's ready to merge - finally

@jacekkalbarczyk jacekkalbarczyk merged commit d5c7690 into CodeForPoznan:master Feb 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants